👌 IMPROVE: Add SQLA migration for parity with Django schema #5097

chrisjsewell · 2021-08-25T07:49:17Z

This PR adds migrations to the SQLAlchemy schema, to make it identical to the Django schema, and adds tests to ensure this parity.

There are three migrations:

Ensures all columns that are to be set with the NOT NULL restriction, are indeed non-null
Sets relevant columns to nullable=False
Changes in a few column types:
- db_dblog.levelname: String(255) -> String(50)
- db_dbsetting.key: String(255) -> String(1024)
- db_dbsetting.description: String(255) -> Text()
Adds/renames all indexes and constraints to be identical to those in the django schema

tests/backends/test_schema_parity.py then tests the parity of the schema columns (name, type, nullable), primary key constraints, unique constraints and indexes.

This also required a regression fix for #3582, which had actually broken the sqlalchemy migration mechanism: instead of carrying out each migration in a separate transaction, this had caused all migrations to be carried out in the same transaction. This caused cannot ALTER TABLE "db_xxx" because it has pending trigger events errors, where changes in one migration were not "flushed" to the database on a transaction close.

In renaming indexes/constraints the sqlalchemy models now adopt the auto-generated names from django (see https://github.com/django/django/blob/2f33217ea2cad688040dd6044cdda946c62e5b65/django/db/backends/base/schema.py#L989).

In order to control the naming, all indexes and constraints are now specified specifically in the sqlalchemy models (in __table_args__), as opposed to being auto-generated via the index and unique options on the relevant Column.

It is also of note, that due to not taking into account https://code.djangoproject.com/ticket/23577, some of the django migrations were not properly written to rename index/constraint names, and so some names do not actually mirror their column names, for example db_dbgroup_name_... instead of db_dbgroup_label_....

Finally, the database default for repository_metadata was removed, since this is not possible to set in django (see https://code.djangoproject.com/ticket/470), and the DbNode.reposotry_metadata properties have been modified, to ensure they return an empty dict, rather than None.

closes #2303, and a step towards addressing #4985

for more information, see https://pre-commit.ci

codecov · 2021-12-09T02:56:00Z

Codecov Report

Merging #5097 (8a77436) into develop (e4ce3f4) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #5097      +/-   ##
===========================================
+ Coverage    81.90%   82.01%   +0.11%     
===========================================
  Files          530      533       +3     
  Lines        38009    38230     +221     
===========================================
+ Hits         31128    31350     +222     
+ Misses        6881     6880       -1

Flag	Coverage Δ
django	`77.06% <23.51%> (-0.41%)`	⬇️
sqlalchemy	`76.35% <99.65%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...jsite/db/migrations/0048_computer_name_to_label.py	`100.00% <ø> (ø)`
...ations/versions/1feaea71bd5a_migrate_repository.py	`83.59% <ø> (ø)`
aiida/backends/sqlalchemy/manager.py	`85.46% <100.00%> (+0.55%)`	⬆️
...igrations/versions/1de112340b16_django_parity_1.py	`100.00% <100.00%> (ø)`
...igrations/versions/1de112340b17_django_parity_2.py	`100.00% <100.00%> (ø)`
...igrations/versions/1de112340b18_django_parity_3.py	`100.00% <100.00%> (ø)`
...sions/7536a82b2cc4_add_node_repository_metadata.py	`100.00% <100.00%> (ø)`
aiida/backends/sqlalchemy/models/authinfo.py	`88.89% <100.00%> (+0.43%)`	⬆️
aiida/backends/sqlalchemy/models/comment.py	`96.43% <100.00%> (+0.28%)`	⬆️
aiida/backends/sqlalchemy/models/computer.py	`89.66% <100.00%> (+0.77%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4ce3f4...8a77436. Read the comment docs.

chrisjsewell · 2021-12-10T00:45:27Z

Ok this is coming along now.

One thing to note is that django auto-generates names for indexes that have these partial-uuids as part of the name, e.g. db_dbcomment_user_id_8ed5e360, with a few also having "wrong" names, e.g. db_dbgroup_name_66c75272 is an index for the label column
(see #4985 (comment))
I don't think in django there is any way to directly specify the index names (as sqlalchemy allows)

For parity, we want these names to be exactly the same so, assuming there is no possible django migration to change the names, I guess the migration here should rename all indexes.

Note, alembic does not allow for an actual rename operation: sqlalchemy/alembic#246, so we would need to drop the indexes then recreate them

chrisjsewell · 2021-12-13T12:46:23Z

Analysis of previous unique constraints:

$ verdi -p a-import-django devel run-sql "SELECT tbl.relname,c.conname,c.contype,c.conkey FROM pg_constraint AS c INNER JOIN pg_class AS tbl ON tbl.oid = c.conrelid WHERE c.contype='u'  ORDER BY tbl.relname,c.conname;"
('db_dbauthinfo', 'db_dbauthinfo_aiidauser_id_dbcomputer_id_777cdaa8_uniq', 'u', [5, 6])
('db_dbcomment', 'db_dbcomment_uuid_49bac08c_uniq', 'u', [2])
('db_dbcomputer', 'db_dbcomputer_name_key', 'u', [3])
('db_dbcomputer', 'db_dbcomputer_uuid_f35defa6_uniq', 'u', [2])
('db_dbgroup', 'db_dbgroup_name_type_12656f33_uniq', 'u', [3, 4])
('db_dbgroup', 'db_dbgroup_uuid_af896177_uniq', 'u', [2])
('db_dbgroup_dbnodes', 'db_dbgroup_dbnodes_dbgroup_id_dbnode_id_eee23cce_uniq', 'u', [2, 3])
('db_dblog', 'db_dblog_uuid_9cf77df3_uniq', 'u', [10])
('db_dbnode', 'db_dbnode_uuid_62e0bf98_uniq', 'u', [2])
('db_dbsetting', 'db_dbsetting_key_1b84beb4_uniq', 'u', [2])
('db_dbuser', 'db_dbuser_email_30150b7e_uniq', 'u', [5])

$ verdi -p a-import-sqla devel run-sql "SELECT tbl.relname,c.conname,c.contype,c.conkey FROM pg_constraint AS c INNER JOIN pg_class AS tbl ON tbl.oid = c.conrelid WHERE c.contype='u'  ORDER BY tbl.relname,c.conname;"
('db_dbauthinfo', 'db_dbauthinfo_aiidauser_id_dbcomputer_id_key', 'u', [2, 3])
('db_dbcomment', 'db_dbcomment_uuid_key', 'u', [2])
('db_dbcomputer', 'db_dbcomputer_label_key', 'u', [3])
('db_dbcomputer', 'db_dbcomputer_uuid_key', 'u', [2])
('db_dbgroup', 'db_dbgroup_label_type_string_key', 'u', [3, 4])
('db_dbgroup', 'db_dbgroup_uuid_key', 'u', [2])
('db_dbgroup_dbnodes', 'db_dbgroup_dbnodes_dbgroup_id_dbnode_id_key', 'u', [3, 2])
('db_dblog', 'db_dblog_uuid_key', 'u', [10])
('db_dbnode', 'db_dbnode_uuid_key', 'u', [2])
('db_dbsetting', 'db_dbsetting_key_key', 'u', [2])

chrisjsewell · 2021-12-14T05:47:41Z

Note it is non possible to set database level defaults in django (https://code.djangoproject.com/ticket/470), so I am removing the server_default on repository_metadata in sqlalchemy.
In django I made the repository_metadata non-nullable, which is complicit with the value returned by the BackendNode, which is expected to always be a dict.
Maybe double-check that this is indeed the case during the migration (that values are set as {})

chrisjsewell · 2021-12-14T06:25:24Z

another issue with django, it didn't rename the indexes & constraints when applying aiida/backends/djsite/db/migrations/0021_dbgroup_name_to_label_type_to_type_string.py, aiida/backends/djsite/db/migrations/0030_dbnode_type_to_dbnode_node_type.py and aiida/backends/djsite/db/migrations/0048_computer_name_to_label.py (see https://code.djangoproject.com/ticket/23577), so they are all named by the old field names 🤦

In aiidateam#3582, SQLA migrations were broken, by starting a top-level transaction, instead of letting alembic open per-migration transaction.

sphuber

Have given the code a look now. Will now to try to run the migration locally and test things a bit.

aiida/backends/sqlalchemy/migrations/versions/91b573400be5_prepare_schema_reset.py

aiida/backends/sqlalchemy/models/comment.py

aiida/backends/sqlalchemy/models/computer.py

aiida/backends/sqlalchemy/models/group.py

aiida/backends/sqlalchemy/migrations/versions/1de112340b16_django_parity_1.py

sphuber · 2021-12-17T10:26:46Z

aiida/backends/sqlalchemy/migrations/versions/1de112340b16_django_parity_1.py

+        sa.Column('type', sa.String(255)),
+    )
+
+    op.execute(db_dblink.update().where(db_dblink.c.type.is_(None)).values(type=''))


Already commented on the default setting for the Comment.ctime, but this thing seems way more serious. I wonder if we should collect all default settings that would be problematic if there are really rows out there that have null values. We should then have a "integrity-check" that finds these. I fear that if we just silently set these missing values to a default, the database will be broken at other points. Having a Link without a type is bad...

Well I imagine having them set to None would already be bad, and so going from None -> '' is not going to break the database any more than it was already.
I think in reality the python code should have already stopped this from occurring, but yeh having an integrity-check could be done

Well maybe it would actually be better to have it fail in this case. Assuming that these cases are rare or not existing, we could analyze the problem and provide a proper fix. Tecnhically we would be able to deduce the correct type based on input and output nodes. I think this column is too delicate to be changing silently

Ok, so if I just remove this line then.
Is there any other you think should be definitely removed?

I guess we can think to provide a "fix" under verdi storage for such issues,
but I imagine we can defer that to another PR?

(and any that do get removed, I can add a comment on in the docstring)

Yes, essentially there are these two comments remaining: (1) remove the setting of defaults that should really be considered as grave bugs (such as this one. Please also check if there are others), and (2) if you keep the commented out lines for operations that should have been there in principle but can't due to problems in original schema, add a generic comment in top docstring.

Ok I have removed this default setting, and added a comment to the docstring. I didn't find any other's that would corrupt the database

aiida/backends/sqlalchemy/migrations/versions/1de112340b16_django_parity_1.py

aiida/backends/sqlalchemy/migrations/versions/1de112340b18_django_parity_3.py

chrisjsewell · 2022-01-12T08:50:38Z

O.M.G.
After an inordinate amount of debugging I found the issue and a tentative fix:

So after here:

aiida-core/aiida/backends/sqlalchemy/manager.py

Line 172 in c743a33

inspector = sqlalchemy.inspect(get_scoped_session().bind)

and here:

aiida-core/aiida/backends/sqlalchemy/manager.py

Line 187 in c743a33

setting = get_scoped_session().query(DbSetting).filter_by(key=key).one()

(at least)

you end up within an "idle" transaction:

$ ps x | grep postgres
...
12539   ??  Ss     0:00.01 postgres: aiida_qs_chrisjsewell_3abef2c96e126c712c1d3e256a3590b9 test-sqla9_chrisjsewell_3abef2c96e126c712c1d3e256a3590b9 ::1(51834) idle in transaction

when alembic then tries to alter the table during the migration:

01/12/2022 09:29:17 AM <13504> alembic.runtime.migration: [INFO] Running upgrade 1de112340b16 -> 1de112340b17, Parity with Django backend (rev: 0049),
part 2: Alter columns to be non-nullable and change type of some columns.

it hangs waiting for this idle transaction to close

 7434   ??  Ss     0:00.01 postgres: aiida_qs_chrisjsewell_3abef2c96e126c712c1d3e256a3590b9 test-sqla9_chrisjsewell_3abef2c96e126c712c1d3e256a3590b9 ::1(50629) idle in transaction  
 7435   ??  Ss     0:00.01 postgres: aiida_qs_chrisjsewell_3abef2c96e126c712c1d3e256a3590b9 test-sqla9_chrisjsewell_3abef2c96e126c712c1d3e256a3590b9 ::1(50630) idle  
 7436   ??  Ss     0:00.04 postgres: aiida_qs_chrisjsewell_3abef2c96e126c712c1d3e256a3590b9 test-sqla9_chrisjsewell_3abef2c96e126c712c1d3e256a3590b9 ::1(50631) ALTER TABLE waiting

I don't know why this idle transaction opens, and cannot think how to stop it from opening.
But, in d522b44, if I add:

        transaction = get_scoped_session().get_transaction()
        if transaction:
            transaction.close()

before starting the migration, this closes any current transaction, allowing for the migration to proceed properly

sphuber

Thanks @chrisjsewell I think we can almost merge this. I closed all resolved comments, but there are still a couple outstanding

aiida/backends/sqlalchemy/manager.py

aiida/backends/sqlalchemy/migrations/versions/91b573400be5_prepare_schema_reset.py

chrisjsewell · 2022-01-14T02:58:55Z

Can you please still try to remove this change and so we only rely on the parity migration to set the proper default? Not ideal to be changing a migration that has already been released.

Ok trying reverting in be0c2bf (#5097)
Hopefully now, this isn't a problem, with the fix regarding idle transactions

chrisjsewell · 2022-01-14T03:04:54Z

Ok I just tested a "manual" migration, and all seems to be running as expected now 🎉

Co-authored-by: Sebastiaan Huber <[email protected]>

sphuber

Ok, just found a few more things, some minor, but some very important. I made them all as suggestions, so I think you can just all batch them in one go and we can merge. Think you will agree with all of them

aiida/backends/sqlalchemy/migrations/versions/1de112340b18_django_parity_3.py

aiida/backends/sqlalchemy/migrations/versions/1de112340b17_django_parity_2.py

aiida/backends/sqlalchemy/migrations/versions/1de112340b16_django_parity_1.py

aiida/backends/sqlalchemy/migrations/versions/1de112340b17_django_parity_2.py

aiida/backends/sqlalchemy/migrations/versions/1de112340b18_django_parity_3.py

aiida/backends/sqlalchemy/migrations/versions/1de112340b16_django_parity_1.py

Co-authored-by: Sebastiaan Huber <[email protected]>

sphuber

Thanks @chrisjsewell let's get this show on the road

for more information, see https://pre-commit.ci

chrisjsewell · 2022-01-15T05:26:21Z

cheers for the review!

chrisjsewell mentioned this pull request Aug 25, 2021

Drop Django Backend #4985

Closed

chrisjsewell force-pushed the sqla-migration branch 2 times, most recently from a8b9da7 to 3831363 Compare September 16, 2021 09:09

chrisjsewell added 5 commits September 16, 2021 11:11

Change SQLA models for parity with Django

7a849f0

add migrations

fdfa555

add tests

3ab7065

Update 1de112340b17_django_parity_2.py

969b8b6

Add example index with varchar_pattern_ops

35621dc

chrisjsewell force-pushed the sqla-migration branch from 3831363 to 35621dc Compare September 16, 2021 09:12

chrisjsewell mentioned this pull request Oct 22, 2021

🧪 TESTS: SQLA Migrations -> pytest #5192

Merged

chrisjsewell and others added 6 commits December 8, 2021 13:43

Merge remote-tracking branch 'upstream/develop' into sqla-migration

72a4b9f

[pre-commit.ci] auto fixes from pre-commit.com hooks

cfdfef2

for more information, see https://pre-commit.ci

update initial revision

f94129c

add index migration

cbecb79

[pre-commit.ci] auto fixes from pre-commit.com hooks

95ed9d0

for more information, see https://pre-commit.ci

fix schema diff

effaac2

Merge branch 'develop' into sqla-migration

f5a1388

chrisjsewell added 2 commits December 10, 2021 08:05

Add missing indexes

5719b07

update

7b3ea13

Merge branch 'develop' into sqla-migration

32ab71c

chrisjsewell added 5 commits December 14, 2021 08:27

update

fa55f63

update

3631a8f

fix postgresql_ops

2e0877d

add test

74b3f06

🐛 FIX: SQLA migration regression

e5d8282

In aiidateam#3582, SQLA migrations were broken, by starting a top-level transaction, instead of letting alembic open per-migration transaction.

sphuber requested changes Dec 17, 2021

View reviewed changes

chrisjsewell added 2 commits January 12, 2022 09:36

fix migration freezing

d522b44

move logic higher up

a519eff

chrisjsewell added 3 commits January 13, 2022 03:42

Merge branch 'develop' into sqla-migration

857d998

Merge branch 'develop' into sqla-migration

9df317c

Merge branch 'develop' into sqla-migration

855958f

sphuber requested changes Jan 13, 2022

View reviewed changes

aiida/backends/sqlalchemy/manager.py Show resolved Hide resolved

aiida/backends/sqlalchemy/migrations/versions/91b573400be5_prepare_schema_reset.py Outdated Show resolved Hide resolved

chrisjsewell added 3 commits January 14, 2022 03:50

add comment on the reason for calling transaction.close

959924f

Merge branch 'develop' into sqla-migration

c07f907

Try reverting change to pre v2 migration

be0c2bf

chrisjsewell and others added 4 commits January 14, 2022 12:43

Apply suggestions from code review

f208bcd

Co-authored-by: Sebastiaan Huber <[email protected]>

add note on nullable foreign keys

266809d

Apply suggestions from code review

365461c

Co-authored-by: Sebastiaan Huber <[email protected]>

remove migration of null link type to ''

1037c75

chrisjsewell requested a review from sphuber January 14, 2022 13:23

sphuber requested changes Jan 14, 2022

View reviewed changes

Apply suggestions from code review

734dc28

Co-authored-by: Sebastiaan Huber <[email protected]>

chrisjsewell requested a review from sphuber January 14, 2022 16:39

sphuber previously approved these changes Jan 14, 2022

View reviewed changes

Update test_12_sqla_django_parity.py

708de2f

chrisjsewell dismissed sphuber’s stale review via 708de2f January 15, 2022 01:09

[pre-commit.ci] auto fixes from pre-commit.com hooks

8a77436

for more information, see https://pre-commit.ci

chrisjsewell merged commit ec6fa17 into aiidateam:develop Jan 15, 2022

chrisjsewell deleted the sqla-migration branch January 15, 2022 05:26

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

👌 IMPROVE: Add SQLA migration for parity with Django schema #5097

👌 IMPROVE: Add SQLA migration for parity with Django schema #5097

chrisjsewell commented Aug 25, 2021 •

edited

Loading

codecov bot commented Dec 9, 2021 •

edited

Loading

chrisjsewell commented Dec 10, 2021

chrisjsewell commented Dec 13, 2021

chrisjsewell commented Dec 14, 2021 •

edited

Loading

chrisjsewell commented Dec 14, 2021 •

edited

Loading

sphuber left a comment

sphuber Dec 17, 2021

chrisjsewell Dec 17, 2021

sphuber Jan 13, 2022

chrisjsewell Jan 14, 2022

chrisjsewell Jan 14, 2022

sphuber Jan 14, 2022

chrisjsewell Jan 14, 2022

chrisjsewell commented Jan 12, 2022

sphuber left a comment

chrisjsewell commented Jan 14, 2022

chrisjsewell commented Jan 14, 2022

sphuber left a comment

sphuber left a comment

chrisjsewell commented Jan 15, 2022

👌 IMPROVE: Add SQLA migration for parity with Django schema #5097

👌 IMPROVE: Add SQLA migration for parity with Django schema #5097

Conversation

chrisjsewell commented Aug 25, 2021 • edited Loading

codecov bot commented Dec 9, 2021 • edited Loading

Codecov Report

chrisjsewell commented Dec 10, 2021

chrisjsewell commented Dec 13, 2021

chrisjsewell commented Dec 14, 2021 • edited Loading

chrisjsewell commented Dec 14, 2021 • edited Loading

sphuber left a comment

Choose a reason for hiding this comment

sphuber Dec 17, 2021

Choose a reason for hiding this comment

chrisjsewell Dec 17, 2021

Choose a reason for hiding this comment

sphuber Jan 13, 2022

Choose a reason for hiding this comment

chrisjsewell Jan 14, 2022

Choose a reason for hiding this comment

chrisjsewell Jan 14, 2022

Choose a reason for hiding this comment

sphuber Jan 14, 2022

Choose a reason for hiding this comment

chrisjsewell Jan 14, 2022

Choose a reason for hiding this comment

chrisjsewell commented Jan 12, 2022

sphuber left a comment

Choose a reason for hiding this comment

chrisjsewell commented Jan 14, 2022

chrisjsewell commented Jan 14, 2022

sphuber left a comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

chrisjsewell commented Jan 15, 2022

chrisjsewell commented Aug 25, 2021 •

edited

Loading

codecov bot commented Dec 9, 2021 •

edited

Loading

chrisjsewell commented Dec 14, 2021 •

edited

Loading

chrisjsewell commented Dec 14, 2021 •

edited

Loading